fix(events): carry machine events on the terminal WebSocket, with SSE fallback - #2672

Merge Risk: Moderate · up to a6e2a

Resolve machine-event access after gate closure and prevent overflow handling from closing terminals before merging. Smaller remaining issues can lose pairing notices during transport handoff and leave temporary state after a failed browser launch.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 58.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta…	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Title check		The title names the main change: machine events now travel over the terminal WebSocket, with SSE as fallback. The raw summary and objectives confirm this is the core of the changeset. The title is spe…
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.
Description check		The description includes the required Change and Validation sections. It explains the problem, behavior, fallback and rollback paths, contract changes, test coverage, command results, skipped checks, …

Full details: Title check

Explanation

The title names the main change: machine events now travel over the terminal WebSocket, with SSE as fallback. The raw summary and objectives confirm this is the core of the changeset. The title is specific and follows a conventional commit style, and it is under the usual length limit at 78 characters.

Full details: Description check

Explanation

The description includes the required Change and Validation sections. It explains the problem, behavior, fallback and rollback paths, contract changes, test coverage, command results, skipped checks, and HTTP/WebSocket validation.

Full details: Docstring Coverage

Explanation

Docstring coverage is 58.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 12 functions across 19 files. (2 skipped: 2 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Recent review info

Run configuration

• Source mode: Auto
• Repository: devswha/ovis
• Branch: fix/machine-events-ws
• File: .coderabbit.yaml
• Commit: a6e2aec
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: f2e0c545-453e-4d35-b240-48b13bbde292

Commits

Reviewing files that changed from the base of the PR and between 4bc043d and a6e2aec.

Files selected for processing: 21

• changelog.d/multiplex-machine-events.md
• docs/development.md
• scripts/ci-browser.sh
• scripts/ci-browser.test.ts
• scripts/machine-events-browser-regression.ts
• server/core/connections.test.ts
• server/core/connections.ts
• server/core/socket.ts
• server/dispatch.ts
• server/machine-api.ts
• server/machine-events.contract.test.ts
• server/machine-events.ts
• shared/protocol.ts
• site/demo/transport.ts
• src/App.tsx
• src/lib/machineEventStream.test.ts
• src/lib/machineEventStream.ts
• src/lib/machineSessionContext.tsx
• src/lib/useMachineRoster.ts
• src/lib/ws.test.ts
• src/lib/ws.ts

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review.

Review round on a6e2aec (omo code review: CLEAR; Codex Terra xhigh, adversarial). Fixed in 8ce86a5:

1. HIGH (reproduced, fixed): the machine-events-ended notice could itself close the terminal. It went through the budgeted send, so with fewer than ~30 bytes left in the budget the socket closed with 4008. Repro at a6e2aec (setMachineEvents on a socket with OUTPUT_HARD_BYTES - 10 queued): [{"code":4008,...}]; at 8ce86a5: ["machine-events-ended"]. The notice is now sent past the budget. The connections.test.ts case covers that boundary and fails on the old code.
2. HIGH (Codex; mitigated, not reproduced end to end): machine-event backlog could pause or stop a healthy terminal. Machine frames count in the socket backlog that reconcileOutput measures, so frames short of the hard budget could still push it past the 256 KiB high-water mark and pause the shared pty. Machine events are now held to OUTPUT_LOW_BYTES (64 KiB) of queue on a socket that already has a backlog; past that the subscription ends and the page uses SSE. A frame sent onto an empty queue still goes, so the remaining case is one large frame (in practice a remote PC's snapshot), on the same footing as the session snapshot this socket already carries.
3. MEDIUM (fixed): StrictMode could close the roster's live local socket. MachineSessionsProvider closed a registry it was handed; it now closes only the one it creates.
4. MEDIUM (not fixed, not reproduced): transient events can be lost across a transport handoff. SSE closes before the server has handled the WS subscribe, and a disconnect waits up to 5 s before SSE opens. The initial documents (roster, preferences, seen) resync state; only a pairing notice or a pane message emitted in that window of a few milliseconds would be missed, which is also true of SSE's own reconnects on main. Left as is; an overlap with event-ID dedupe would be the fix if it shows up.

Access checks traced by Codex (bridge exclusion, watch-device authority, revocation, open-LAN pairing suppression, browser/device self-filter) found no regression. bun run check fast is green at 8ce86a5 (10,893 pass, 0 fail).

omo review of 329c934: BLOCK with two HIGH and one MEDIUM, all reproduced. Fixed in 8262a74 (the last fix round):

1. HIGH (reproduced, fixed): signing out left the local socket open. The sessions registry now lives in App, so the sign-in gate no longer unmounted the provider that used to close every socket; the socket kept its old authority behind the gate and the next sign-in reused it. Locking now closes every session (useCloseSessionsWhenLocked, tested in machineSessionsLock.hook.test.ts, which fails without the effect).
2. HIGH (reproduced, fixed): a large machine event on an empty queue could get the terminal stopped. A roster carries every PC's SessionSnapshot. omo's case, a 205 KB roster followed by 70 KiB of output, got its output window's verdict stop at 4.0 s, and reconcileOutput ended the client. A connection that has queued a machine event now gets the time it needs to cross the slowest budgeted link (32 KiB/s, capped at 15 s, as a joiner's replay gets) before a stall stops it; until then it pauses. A backlog at the hard budget still stops it. Same scenario at 8262a74: stop is held until 8.3 s, by which time the roster has crossed and the output's ACKs reset the stall clock. A flat 64 KiB cap would have sent every page with a large roster back to per-tab SSE, the bug this PR fixes.
3. MEDIUM (reproduced, not fixed): the event that overflows is dropped. When a machine event would exceed 64 KiB of backlog, the subscription ends, the page moves to SSE and gets the full documents again, but a one-off event (a device-paired notice or a relayed pane exit) emitted at that moment is lost, as on an SSE reconnect on main. A fix needs a replay or an ID'd overlap across transports; left as a follow-up. A device pairing still raises its push notification.

bun run check fast is green at 8262a74 (10,895 pass, 0 fail).

@coderabbitai review

omo review of 8262a74: BLOCK on one MEDIUM, no HIGH. Both HIGHs from the previous round are confirmed fixed (sign-out closes the sockets; the stall grace holds for the reproduced 205 KB roster).

• MEDIUM (reproduced at model level, not fixed here): a roster over about 416 KiB can outlast the 15 s grace cap at the budgeted 32 KiB/s link and still get the terminal stopped. Filed as Machine events: a roster larger than the stall grace allows can still stop terminal output #2680, with the two candidate fixes. This was the last fix round, so it is not changed in this PR.
• MEDIUM (unreproduced): WebSocket machine events are not charged to the shared SSE event-stream budget. Terminal snapshots already get the same one-oversized-frame allowance on main. Noted in the issue.
• LOW (unreproduced): a client.send() result of 0 for a machine event closes the socket instead of calling overflow(); this needs a frame past Bun's backpressure limit (about 65 MiB). Noted in the issue.

omo review of 461b428: BLOCK on one HIGH introduced by the previous fix round, now fixed in the next commit.

• HIGH (reproduced, fixed): every machine event renewed the stall grace. With events at least every 2 s, a client that never acknowledges kept the shared pty paused for every viewer indefinitely, where main stops it after 10 s. A grace is now granted only when the event actually left bytes queued, one at a time, with a stall's length (2 s) between grants. connections.test.ts sends a small event every 1.5 s for a minute and asserts the client is open to a stall stop at least every 10 s; it fails with the old renewal. The 205 KB roster case still holds its stop until 8.3 s.
• MEDIUM (unreproduced): a roster too large to cross within the 15 s cap; already Machine events: a roster larger than the stall grace allows can still stop terminal output #2680.
• LOW (unreproduced, also true of SSE on main): opened() fires before the first preferences event; and the SSE-to-WS handoff can lose a pairing notice sent within one round trip (see the earlier review comment).

CI's browser lane at 8262a74 also failed: wake-lock-regression (in ui-contexts) and three other fixtures abort, rewrite or observe /api/machines/events to control the roster. Fixed in 461b428 by pinning those fixtures' in-process servers to SSE (OVIS_MACHINE_EVENTS_WS=0); the socket transport keeps its own contract tests and machine-events-browser-regression.ts. Locally ui-contexts, recent-folders, file-viewer, preferences-sync and machine-events-browser all pass.

@coderabbitai review

omo review of 5558204: BLOCK on MEDIUMs only, no HIGH. It confirms the earlier fixes hold: the pairing filter matches SSE, bridge tokens get no document, revocation closes the socket, sign-out closes every socket, and the ending notice cannot cause a stall close.

• MEDIUM (reproduced at model level): a small event shortly before a large roster leaves the roster without a stall grace of its own (they are spaced to stop a stream of events from holding the pty paused). Added to Machine events: a roster larger than the stall grace allows can still stop terminal output #2680, with why the obvious fix reopens the earlier HIGH.
• MEDIUM (reproduced, roster over about 416 KiB): the 15 s grace cap; already Machine events: a roster larger than the stall grace allows can still stop terminal output #2680.
• LOW: the machine-events capability is read from untrusted features frames too; it only picks the transport and the server ignores a subscribe it has no document for.

Per the takeover brief this was the last fix round; these stay as #2680.

CodeRabbit reviewed this PR once, at a6e2aec (four findings, all answered and resolved above). Both later @coderabbitai review requests (for 8262a74 and 5558204) came back "Review rate limited", so no CodeRabbit review covers 8ce86a5 onward. Those commits were reviewed by the omo code reviewer and Codex Terra instead; their findings and how each was resolved are in the comments above. Not waiting on CodeRabbit.

@devswha Understood. CodeRabbit’s review covers a6e2aecf, not 8ce86a52 or later commits. The comments document the later omo and Codex Terra reviews and the remaining follow-ups in #2680. I will not start another review unless you request one.

_{You are interacting with an AI system.}

omo review of bf942af (main merged in, no code change since 5558204): BLOCK, again on the large-roster stall grace (#2680), now with a sharper repro:

• A roster sent onto an empty queue gets a grace capped at 15 s, without OutputWindow.join's + crossingMs(large): at the 32 KiB/s budget floor with 256 KiB of output behind it, a 480 KiB roster is stopped at 15.1 s while its first ACK could arrive at 16.0 s.
• New: a roster of about 768 KiB–1 MiB plus 256 KiB of output passes OUTPUT_HARD_BYTES at once and the socket is stopped immediately, since only frames over 1 MiB are excused.
• Unreproduced: opened() fires before the first preferences document (same order as SSE's onopen); no cross-connection budget for queued machine events (needs a ~32 MiB roster); the useRosterState cleanup closes the registry under StrictMode in development, which recovers.

The fix both reviews converge on is to cap every machine event at OUTPUT_LOW_BYTES, ending the subscription (machine-events-ended, the page moves to SSE) for a larger one even on an empty queue. That would make the stall grace unnecessary and close all of #2680, at the cost of per-tab SSE for pages whose roster is over 64 KiB. Left for the maintainer's call; no change pushed.

Maintainer decision applied in d495405: machine events on the terminal socket are capped at OUTPUT_LOW_BYTES (64 KiB). An event that would put more than that on the socket, counting what is queued already, or that the socket refuses, ends the subscription (machine-events-ended) and the page takes machine events over SSE; it is never queued ahead of terminal output and never closes the socket with 4008. The stall grace is removed, which closes #2680. connections.test.ts covers a 900 KiB roster on an empty queue followed by 256 KiB of output: the terminal stays open and the page gets machine-events-ended. Limit, now in the PR body and docs: a page whose roster is over 64 KiB keeps per-tab SSE, as on main.

omo review of d495405 (with Codex Terra): CLEAR, no HIGH. LOWs: the 31-byte ending notice bypasses the budget, so on a relayed socket queued within ~31 bytes of 1 MiB it can trip the relay socket's own 4008 (that socket's next terminal frame would be closed the same way, on main too); the machine-events capability is read from untrusted features frames (transport choice only); the local socket sets a stream-key cookie and stays connected while a remote PC is selected (intended). bun run check fast green at d495405 (10,906 pass, 0 fail).

Merge note: this changes the terminal WebSocket contract (machine events negotiated per reconnect, SSE kept as fallback). CodeRabbit reviewed a6e2aec (4 threads resolved) and was rate-limited after that, so the review agents stand in for later heads: omo via dori review (CLEAR at 28f17e5; two unreproduced MEDIUMs that match main's SSE ordering), Codex Terra xhigh across several rounds (every reproduced HIGH fixed with a test: SSE fallback on stall/server end, sign-out closes sockets, a terminal socket is never closed by machine events). Final design caps machine events at OUTPUT_LOW_BYTES; larger rosters keep per-tab SSE. Based on #2646 by @cheol-95 - thank you!